Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved hashing algorithm in luaS_newlstr #1168

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

zarkash-aws
Copy link
Contributor

Overview

This PR introduces the use of MurmurHash3 as the hashing function for Lua's luaS_newlstr function, replacing the previous simple hash function. The change aims to improve performance, particularly for large strings.

Changes

Implemented MurmurHash3 algorithm in lstring.c
Updated luaS_newlstr to use MurmurHash3 for string hashing

Performance Testing:
Test Setup:

  1. Ran a valkey server
  2. Loaded 1000 keys with large values (100KB each) to the server using a Lua script
local numKeys = 1000

for i = 1, numKeys do
    local key = "large_key_" .. i
    local largeValue = string.rep("x", 1024*100)
    redis.call("SET", key, largeValue)
end
  1. Used a Lua script to randomly select and retrieve keys
local randomKey = redis.call("RANDOMKEY")
local result = redis.call("GET", randomKey)
  1. Benchmarked using valkey-benchmark:
    ./valkey-benchmark -n 100000 evalsha c157a37967e69569339a39a953c046fc2ecb4258 0

Results:

A Unstable This PR Change
Throughput 6,835.74 requests per second 17,061.94 requests per second +150% increase
Avg Latency 7.218 ms 2.838 ms -61% decrease
Min Latency 3.144 ms 1.320 ms -58% decrease
P50 Latency 8.463 ms 3.167 ms -63% decrease
P95 Latency 8.863 ms 3.527 ms -60% decrease
P99 Latency 9.063 ms 3.663 ms -60% decrease
Max Latency 63.871 ms 55.327 ms -13% decrease

Summary:

  • Throughput: Improved by 150%.
  • Latency: Significant reductions in average, minimum, and percentile latencies (P50, P95, P99), leading to much faster response times.
  • Max Latency: Slightly decreased by 13%, indicating fewer outlier delays after the fix.

Signed-off-by: Shai Zarka <zarkash@amazon.com>
Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.63%. Comparing base (36d438b) to head (4c749ce).
Report is 9 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1168      +/-   ##
============================================
- Coverage     70.69%   70.63%   -0.06%     
============================================
  Files           114      114              
  Lines         61693    61740      +47     
============================================
+ Hits          43611    43613       +2     
- Misses        18082    18127      +45     

see 11 files with indirect coverage changes

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

Are Lua tables using this hash function too? A Lua script that just creates large lua tables and looks up elements in it would benefit much too, right?

Lua is a vendored dependency. Having patches to this code makes it harder to update the dependency. I'm not sure we will ever update this though, because later Lua versions are not 100% compatible with 5.1, but in general.

In deps/README.md, it's described how to update each dep. We should mention there that we have changed Lua's hash function.

deps/lua/src/lstring.c Outdated Show resolved Hide resolved
zarkash-aws and others added 2 commits October 15, 2024 16:36
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Signed-off-by: zarkash-aws <zarkash@amazon.com>
Signed-off-by: Shai Zarka <zarkash@amazon.com>
@madolson madolson added performance release-notes This issue should get a line item in the release notes labels Oct 15, 2024
deps/lua/src/lstring.c Outdated Show resolved Hide resolved
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
deps/lua/src/lstring.c Outdated Show resolved Hide resolved
deps/README.md Outdated Show resolved Hide resolved
Minor improvements.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
deps/lua/src/lstring.c Outdated Show resolved Hide resolved
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
@madolson madolson merged commit 06cfe2c into valkey-io:unstable Oct 15, 2024
46 checks passed
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Oct 20, 2024
**Overview**

This PR introduces the use of
[MurmurHash3](https://en.wikipedia.org/wiki/MurmurHash) as the hashing
function for Lua's luaS_newlstr function, replacing the previous simple
hash function. The change aims to improve performance, particularly for
large strings.

**Changes**

Implemented MurmurHash3 algorithm in lstring.c
Updated luaS_newlstr to use MurmurHash3 for string hashing

**Performance Testing:**
Test Setup:

1. Ran a valkey server
2. Loaded 1000 keys with large values (100KB each) to the server using a
Lua script
```
local numKeys = 1000

for i = 1, numKeys do
    local key = "large_key_" .. i
    local largeValue = string.rep("x", 1024*100)
    redis.call("SET", key, largeValue)
end
```
3. Used a Lua script to randomly select and retrieve keys
```
local randomKey = redis.call("RANDOMKEY")
local result = redis.call("GET", randomKey)
```
4. Benchmarked using valkey-benchmark:
`./valkey-benchmark -n 100000 evalsha
c157a37967e69569339a39a953c046fc2ecb4258 0`

Results:

A | Unstable | This PR | Change
-- | -- | -- | --
Throughput | 6,835.74 requests per second | 17,061.94 requests per
second | **+150% increase**
Avg Latency | 7.218 ms | 2.838 ms | **-61% decrease**
Min Latency | 3.144 ms | 1.320 ms | **-58% decrease**
P50 Latency | 8.463 ms | 3.167 ms | **-63% decrease**
P95 Latency | 8.863 ms | 3.527 ms | **-60% decrease**
P99 Latency | 9.063 ms | 3.663 ms | **-60% decrease**
Max Latency | 63.871 ms | 55.327 ms | **-13% decrease**

Summary:
* Throughput: Improved by 150%.
* Latency: Significant reductions in average, minimum, and percentile
latencies (P50, P95, P99), leading to much faster response times.
* Max Latency: Slightly decreased by 13%, indicating fewer outlier
delays after the fix.

---------

Signed-off-by: Shai Zarka <zarkash@amazon.com>
Signed-off-by: zarkash-aws <zarkash@amazon.com>
Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance release-notes This issue should get a line item in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants